Skip to content

Click casting: fix frames that never registered, and the pinned-frame bugs that made it fire - #223

Open
Krathe82 wants to merge 14 commits into
DanderBot:mainfrom
Krathe82:fix/clickcast-frame-registration
Open

Click casting: fix frames that never registered, and the pinned-frame bugs that made it fire#223
Krathe82 wants to merge 14 commits into
DanderBot:mainfrom
Krathe82:fix/clickcast-frame-registration

Conversation

@Krathe82

@Krathe82 Krathe82 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What this is

Click casting silently dying until the user reloads, plus the pinned-frame faults
found alongside it.

The reports all sounded different — "my own frame stopped working", "my binds died
mid-key", "my pinned frames vanished", "it only happens on pinned frames" — and
they were mostly one bug with several faces, sitting behind a handful of smaller
ones. This fixes the shared cause, closes every remaining way hover binds can go
dead, and repairs the pinned-frame bugs that were making the whole thing fire far
more often than it otherwise would.

No setting was added, renamed or removed. Behaviour is unchanged except where it
was broken.

The root cause

Affected frames were never registered for click casting at all — no hooks, no
bindings. Hovering one produced nothing and a bound key fell straight through to
the action bar, casting whatever sat in that slot. That is why it read as "my heal
bind cast Holy Nova" rather than as a binding pointing somewhere wrong.

DF publishes frames into the shared ClickCastFrames table and a metatable
registers each new entry. Lua's __newindex fires only for keys the table does
not already hold
— one chance per frame — and the metamethod spent that chance
before it could use it:

__newindex = function(t, frame, enabled)
    rawset(t, frame, enabled)                   -- key now exists, chance spent
    ...
    elseif clickCastFrameEligible(frame) then   -- requires a unit
        CC:RegisterFrame(frame)

Header children are created before the header assigns their units — Frames/Init.lua
says so itself: "the header pre-creates children but units aren't set until group
members actually appear."
So a fresh child was rawset, refused for having no unit,
and every later write bypassed the metamethod entirely. The frame stayed
unregistered for the session while dfClickCastRegistered reported success.

ScanForThirdPartyFrames would have repaired it, but it runs 1s and 3s after setup
— login only. That is why a reload was the only cure.

Pinned frames took the brunt because a party↔raid mode change recreates all 40
children mid-session, far outside that window. It was never specific to pinned
frames or to the player's own unit: any header child that appears without a unit
and gains one later was affected. A user who pins themselves simply meant their own
frame was in the recreated set.

Fixed twice over. CC:EnsureRegistered is now the single entry point and
remembers a frame that is not eligible yet instead of dropping it; and our own
frames no longer wait for a unit at all, because nothing in the hover-bind path
reads one — the snippet is owner:SetBindingClick(true, key, self, virtualBtn),
the unit resolves at click time, and applicability is decided by a flag set before
registration is attempted. That removes the window rather than shortening it: by
the time a unit arrives, in combat or not, there is nothing left to do.

Why that mattered most in combat

RegisterFrame defers under lockdown. So a frame that first gained its unit
during a fight could not be wrapped until combat ended — no keyboard binds for
the rest of that fight. Arming at creation is what makes the worst case
bounded instead of unbounded.

Everything else that could kill a bind

Found by a full structural audit of the click-casting module rather than by
report, and each one is a real dead-key path:

  • Mid-sweep. ApplyBindings walks every registered frame in batches. Frames
    processed early held the outgoing snippet while their outgoing attributes were
    already erased, so those keys pointed at cleared attributes — dead and stolen
    from the action bar — for the rest of the sweep, or the whole fight if combat
    interrupted it. Snippets are now rebuilt inline per frame.
  • Mid-hover. The sweep wipes the header, killing binds on the frame under the
    cursor. The repair only ran at the tail of the batch walker, over an arbitrary
    iteration order, and fell back to whichever frame it thought was hovered — in one
    capture all five of its successes landed on a different frame than the one just
    cleared. The hovered frame is now processed first and reasserted explicitly.
  • At login. RegisterEvents is reached from inside the PLAYER_ENTERING_WORLD
    dispatch, so our own frame never received that event. Nameplate registration, the
    zone-in repair, and the cold-start profile resolve — the belt added for "none of
    my binds work in my first arena of the day" — were all absent until the first zone
    change.
  • Nameplates. register and unregister could both hold the same frame, with
    drain order alone picking the winner. On Blizzard's recycled nameplate pool the
    same frame is legitimately removed and re-added within one fight, so the drain
    tore down plates showing live units. Queueing one job now cancels its opposite.
  • Toggling in combat. SetEnabled skipped the header attribute the secure
    snippet reads, with no deferral — so enabling click casting mid-fight left every
    hover a silent no-op while the UI reported it working.
  • Two data faults. A binding with no enabled field was read three different
    ways and could kill a key outright; and a key carrying only macro bindings
    produced no macro at all, because every clause builder requires a spell name.

Third-party frames

Registering a foreign frame rewrites its click behaviour, so getting this wrong
damages another addon's UI:

  • Unnamed frames are now refused. SetBindingClick resolves its target through
    GetName(), so an anonymous frame can never be a hover-bind target — but
    registering one was destructive rather than useless. The Blizzard-clear runs early
    with no name check and wiped type1/type2 and every modifier variant, while all
    three functions that would have installed our bindings bail on the missing name
    later. The frame came out with no left-click target, no right-click menu, and
    nothing in their place.
  • unit1/unit2 and mousewheel state are captured and restored. Both were
    overwritten and never put back, so a frame relying on per-button unit overrides
    lost that behaviour, and a frame that shipped with the wheel disabled started
    swallowing scroll events.
  • The opt-out is honoured again. ClickCastFrames[frame] = false is the
    documented unregister, and it never unregistered anything — for any frame already
    registered the key exists, so the write lands with no metamethod. DF kept its
    bindings on frames whose owner had explicitly taken them back.
  • Two silent killers: the pre-existing-entries loop bypassed the eligibility gate
    entirely (the gate that exists because a toy button had its type1 replaced), and
    a misplaced issecretvalue guard could throw inside a timer callback and take out
    all third-party registration for the session from one error at login.

Both retry and opt-out are reconciled by comparing the two tables rather than by
watching writes, since the metatable structurally cannot report either.

Pinned frames

Two independent bugs, both of which wrote to saved config — so the pins were
gone until something re-added them, not merely hidden.

  • Pruned against a roster that had not arrived. The existing guard only caught
    GetNumGroupMembers() == 0, but on a party→raid transition the member count goes
    live before GetRaidRosterInfo resolves, and the raid branch of GetGroupRoster
    has no player fallback — so the count passed while the roster was still completely
    empty and every auto-added pin was pruned against it. Field log: a set logged
    2 players in set, 0 valid, then 0 players in set one second later, then the
    same two members back 21 seconds on. They had been present the whole time.
  • Pruned against a role that had not arrived. The removal pass read a map that
    coerces "NONE" to "DAMAGER", so during role assignment a pinned tank briefly
    read as DPS and a tanks-only set deleted them.

manualPlayers was protected from both, which is exactly why only people using the
auto-add role filters ever saw this. Neither path can now act on unresolved data.

Separately, a pinned frame could be adopted as DF.playerFrame. That pointer is
not a generic "whichever frame holds the player" — around fifty consumers treat it as
the party player frame, SecureSort uses it as party slot 0 and sets secure paths
and swap refs on it, and UpdateAllFrames drives its unit watch and click-cast
registration from it. A user who pinned themselves and hid self from party frames had
all of that pointed at a pinned frame. The sibling write fifty lines above already
guards this case; this one never got the same treatment.

Diagnostics

Not a feature — the reason this took as long to find as it did.

A sweep emitted three INFO lines per frame, so with a full set of third-party unit
frames registered a single sweep wrote ~1,770 entries, and sweeps fire on roster churn. At maxLines = 10000 the log
retained under three minutes and was evicted by its own noisiest writer: two
consecutive attempts to capture this bug came back holding nothing but sweep chatter,
one of them having flushed the reload marker with it. Now one summary line per sweep,
with an INTERRUPTED variant when combat aborts it. All warnings are untouched.

Also removed ~75 lines that could never run — SetupHovercastButtonAttributes named
its slots in one namespace while the bindings that reach that button are installed in
another, so nothing ever resolved to what it wrote, and it leaked an attribute-driver
list that grew on every apply.

Verification

Every file parses; secure-snippet token check, use-before-declaration scan and an AST
dangling-call pass all clean.

Play-tested across LFR, repeated group joins and party↔raid mode changes:

key presses landing on a real macro 155 / 155 across two runs
HOVER BUT NO KB BINDINGS / BINDINGS VANISHED / EMPTY SNIPPET 0
real stuck binds 0 (every logged case was a spurious insecure leave, correctly ignored)
a sweep interrupted by a pull mid-walk no dead keys followed it

Two fixes were caught working in the wild: Reconcile: registered 2 frame(s) that became eligible fired in both runs, rescuing third-party frames that would have been
lost for the session; and the hover reassert repeatedly landed on the frame that had
actually just been cleared.

Worth a reviewer's attention

  • The registration change alters when frames enter the registry. Ours are now
    armed at creation, so frames that never receive a unit are registered too and the
    sweep covers more of them. The sweep summary reports frame count and elapsed, so
    the cost is measurable rather than guessed.
  • Known redundancy I introduced: the sweep now rebuilds each frame's snippet
    inline and RefreshKeyboardBindings repeats it at the tail. Idempotent but
    duplicated work. I left the tail call as a backstop rather than remove it, because
    dropping it risks the snippet-less window the inline rebuild was added to close —
    worth a second opinion.
  • Two release paths were nearly deleted and should not be. The OnHide wrap and
    the mouseoverstate driver showed zero clears across three sessions. They were
    instrumented instead of removed, and the driver then fired — with the cursor
    geometrically on the frame at the moment the mouseover unit was lost, exactly the
    case it exists for. Both stay.
  • Validated by reading, not running: nameplate register/unregister collisions,
    the opt-out release path, unnamed-frame refusal, and the two binding-data fixes.
    Each needs a specific setup rather than general play.

Krathe82 added 13 commits August 2, 2026 16:28
Two independent paths could delete auto-added members from a pinned set.
Both write to saved config, so the pin was gone until something re-added it,
and manualPlayers survives both -- which is why this only ever reproduced for
people using the auto-add role filters.

CleanOfflinePlayers pruned against a roster that had not populated yet. The
existing guard only caught GetNumGroupMembers() == 0, but on a party->raid
transition the member count goes live BEFORE GetRaidRosterInfo(i) resolves,
and the raid branch of GetGroupRoster builds its table purely from
GetRaidRosterInfo with no player fallback -- so the count passed while the
roster was still completely empty and every auto-added pin pruned against it.
Now guarded on the roster actually being populated, and partial population
counts as not populated.

Field log (2026-08-01 15:37:26, v4.9.0-alpha.1): "Mode changed from party to
raid" logged "2 players in set, 0 valid" -- the 0 valid IS the empty roster --
and one second later "0 players in set". The tanks returned 21s later once the
roster arrived and the auto-add pass re-added them.

AutoPopulateSet's removal pass read a role map that coerces "NONE" to
"DAMAGER". During role assignment a pinned TANK therefore momentarily reads as
DAMAGER, and a set with only autoAddTanks enabled removed them. The coercion
is correct for the ADD pass and is kept there; the REMOVE pass now reads a
separate map holding only roles the game has actually assigned, so an
unresolved role can no longer drive a deletion.
The BINDINGS STILL ACTIVE line did not say how the frame got its binds, and
the two ways have completely different release paths. A motion OnEnter arms
Blizzard's secure OnLeave; an OnShow claim cannot, because Wrapped_OnLeave
gates on `motion and self:GetAttribute("_wrapentered")` and only the motion
branch of Wrapped_OnEnter sets that attribute. Restricted code cannot set it
either -- HANDLE:SetAttribute rejects any name matching ^_
(RestrictedFrames.lua:523, Gethe live 4383ced3), so a show-claimed frame
depends on OnHide, the next frame's claim, the mouseoverstate driver, or the
insecure backstop.

Without the field a log cannot tell the two apart after the fact, which cost
a wrong reading of a user log on 2026-08-01. Record it and note the
constraint next to the code that depends on it.
ApplyBindings wipes the header, which kills hover keybinds on whatever frame
the cursor is on. ReassertHoverBinds exists to put them back, but it only ran
at the tail of the batch walker -- and the walker yields between batches over
an arbitrary `pairs` order, so a hovered frame landing late in the iteration
stayed dead for the rest of that sweep, with every DF-bound key falling
through to the action bar meanwhile. The reporter pressed their heal bind and
cast the action-bar spell in that slot instead.

Field capture with ElvUI loaded in LFR (2026-08-02): 500 registered frames per
sweep, 240 of them ElvUI's, against the "100-150+" the batching was sized for.
Two sweeps ran seconds apart at ~392 frame-applies each -- 12:37:37-38 in about
a second, then 12:37:49-54 taking about six -- so the worst observed dead
window is ~6s within one sweep.

The hovered frame is now processed first, so its restore happens in the
synchronous first batch. Two details matter:

Its snippet is rebuilt before reasserting. The batch passes
skipKeyboardUpdate, so at that point the frame still carries the OUTGOING
snippet; reasserting without rebuilding would restore the previous profile's
binds and silently cast the wrong spell, which is worse than no bind.

The frame is passed explicitly. ReassertHoverBinds otherwise falls back to
currentHoveredFrame, and in the same capture all five of its successes landed
on a different frame than the one that had just been cleared.

The tail call stays as a backstop for a cursor that moves mid-sweep.
…r frame

A sweep walks every registered frame, and each frame emitted three INFO lines
(entry, ClearBindings, DONE). With ElvUI loaded that registry is ~590 frames,
so a single sweep wrote ~1770 entries in a second or two, and sweeps fire on
roster churn -- seven of them inside three minutes in one capture.

At maxLines = 10000 that meant the log retained under three minutes of
history, and the eviction was driven entirely by its own noisiest writer. Two
consecutive attempts to capture a reported click-casting failure came back
holding nothing but sweep chatter: the hover and PreClick lines around the
failure had been flushed, and in one case the UI Reload marker with them. A
debug log whose loudest writer destroys the evidence is worse than no log.

The per-frame INFO is now gated behind a `quiet` flag that only the sweep
sets, and the sweep emits a single line with frame count and elapsed ms
(plus an INTERRUPTED variant when combat aborts it mid-walk).

The flag is threaded as a parameter rather than held on CC: a suppression flag
that leaked would silently disable logging, which is the exact failure mode
being fixed here. Hovered-frame warnings and every downstream warning are
untouched -- those are rare and are the ones worth keeping.
Root cause of the long-standing "my own frame stops click casting until a
reload" reports, and of click casting dying on pinned frames specifically.

ClickCastFrames' __newindex rawsets the key BEFORE testing eligibility, and
eligibility requires a unit. A header child is created before the header
assigns units -- Frames/Init.lua says so itself: "the header pre-creates
children but units aren't set until group members actually appear." So a fresh
child was rawset (key now present, its one __newindex spent) and then refused
for having no unit. Every later write bypassed the metatable entirely, because
__newindex only fires for keys the table does not already hold.

The frame stayed unregistered for the whole session -- no hooks, no bindings,
every bound key falling through to the action bar -- while
dfClickCastRegistered claimed it had worked. ScanForThirdPartyFrames would
have repaired it, but it runs 1s and 3s after setup, so it only ever covered
login. That is exactly why a reload was the only cure.

Pinned frames took the brunt because a party<->raid mode change recreates all
40 children mid-session, far outside that window.

Field capture (2026-08-02 16:18): a mode change created 40 pinned children,
and the next 15 seconds of hovering and pressing a bound key produced ZERO
OnEnter/OnLeave/PreClick entries. The same actions after a reload logged
normally with the correct macro, and the binding sweep saw 357 frames before
against 375 after.

Fixed in two places:

CC:EnsureRegistered is now the single entry point. When a frame is not yet
eligible it is remembered rather than dropped, and it honours an explicit
opt-out so a deliberately hidden frame is not resurrected. UnregisterFrame
clears any pending entry unconditionally, since a frame can be unregistered
while still only pending.

The unit-assignment path in Headers.lua calls it, which is the moment the
missing precondition actually arrives. DF's own RegisterFrameWithClickCast
calls it too rather than trusting the table write, so a repeat registration
cannot be silently swallowed by the absent metamethod.
The OnAttributeChanged handler assigned DF.playerFrame from any child that
gained unit=player, pinned frames included. DF.playerFrame is not a generic
"whichever frame holds the player" pointer though -- around fifty consumers
treat it as the party player frame specifically. SecureSort uses it as party
slot 0, setting secure paths and swap frame refs on it, and UpdateAllFrames
drives both its unit watch and its click-cast registration from it. A user who
pins themselves and hides self from party frames had all of that pointed at a
pinned frame.

The sibling write fifty lines above already guards this exact case ("Skip for
pinned frames - they must not remove main frame entries"); this one never got
the same treatment.

Also guards the test-mode block that called UnregisterUnitWatch and :Hide on
DF.playerFrame without a nil check. It can legitimately be nil -- the same
handler clears it when a frame gives up unit=player, and with "hide self from
party frames" no main-frame child may ever be assigned that unit -- so this
was already reachable, and narrowing who sets the pointer makes nil more
likely. The neighbouring blocks all test it first.
Removes the last unbounded dead-bind window rather than shortening it.

clickCastFrameEligible refused any frame without a "unit" attribute. Header
children are created before the header assigns units, so registration was
deferred until the unit arrived -- and if it arrived during combat,
RegisterFrame defers under lockdown, so the wrap and the snippet could not be
installed until combat ended. A frame that first gained its unit mid-fight had
no keyboard binds for the REST OF THE FIGHT. That is the shape of the "my binds
stopped working in the middle of a key" reports: not a bind pointing at the
wrong thing, but a frame that was never armed.

The gate was testing a fact the work does not use. Nothing in the hover-bind
path reads the unit: the snippet is
`owner:SetBindingClick(true, key, self, virtualBtn)`, the unit resolves at click
time from the frame's own attribute, and applicability is decided by
dfIsDandersFrame, which is set before registration is attempted. So our frames
are now eligible from creation. By the time a unit lands, in combat or not,
there is nothing left to do. A unit-less frame is hidden by RegisterUnitWatch
and cannot be hovered, so its binds never activate until it holds someone.

Foreign frames keep the strict unit test. That gate was added to stop click
casting taking over buttons that are not unit frames at all (a toy button had
its type1 replaced), and for anything we did not create, carrying a unit is the
only way to tell.

Cost: frames that never receive a unit are now registered too, so the
ApplyBindings sweep covers more of them. The sweep summary line reports frame
count and elapsed, so the real impact is measurable rather than guessed.

Also instruments the two release paths that have never been observed doing
work. dfHideFired counts every OnHide wrap RUN rather than only its clears, and
the OnEnter line now samples it alongside the state-driver count. "Never
cleared" is ambiguous between dead code and correctly-nothing-to-do, and those
have opposite conclusions -- neither path gets deleted on absence alone. This
is the measurement that kept the OnShow claim and removed the state-driver
reclaim.
…__newindex

Fixes a regression introduced earlier today and a long-standing twin of it.

pendingRegistration was WRITE-ONLY. EnsureRegistered parked frames that were
not yet eligible with the comment "the unit-assignment path retries", but that
retry only exists for our own header children, via the explicit hook in
Frames/Headers.lua. Nothing ever iterated the table. A third-party group header
that registers its children before assigning them units had every child parked
and never looked at again -- an entire foreign raid grid silently without click
casting until a reload. That is the same write-only-flag antipattern this work
has been removing, and I added it.

The twin, which predates it: `ClickCastFrames[frame] = false` is the documented
Clique-convention opt-out, and it never unregistered anything. __newindex fires
only for keys the table does not already hold, and the rawset inside it spends
each frame's one chance on the first write -- so for any frame that was ever
registered, the unregister branch is unreachable. DF kept its bindings, wrap and
snippet on frames whose owning addon had explicitly taken them back, for the
rest of the session.

Neither is observable by watching writes, so ReconcileClickCastFrames compares
the two tables instead: register anything parked that has become eligible,
release anything we hold that has since been marked false. Called from
ApplyBindings, which runs on roster churn -- the same churn that creates and
retires these frames. Cheap: the pending table is near-empty in steady state and
the registry walk is a few hundred entries we already own.
Five fixes, all of them cases where binds went dead rather than merely wrong.

The batched sweep left every already-processed frame holding the OUTGOING
snippet while its outgoing type-<virtualBtn> attributes had already been erased,
so the keys that snippet bound pointed at cleared attributes -- dead AND stolen
from the action bar -- for the rest of the sweep, or for the whole fight if
combat interrupted it. The snippet is now rebuilt inline per frame. That costs
nothing: the batch tail already called UpdateFrameBindingAttributes once per
registered frame, so it is the same work moved earlier, and clear-then-rebuild
now happens inside one call with no yield for combat to land in.

The zone settle pass never ran at login. RegisterEvents is reached from inside
the dispatch of PLAYER_ENTERING_WORLD, so our own frame never receives that
event. Nameplate registration, the zone-in binding repair and the cold-start
profile resolve -- the belt added for "no binds in my first arena of the day" --
were all absent until the first zone change. Factored out as ScheduleZoneSettle
and kicked once at init; the timer key keeps it idempotent.

Opposed deferred jobs let DRAIN_ORDER arbitrate instead of the caller. Both
register/unregister and blizzardRegister/blizzardUnregister could hold the same
payload, and the drain order alone decided the winner. On Blizzard's recycled
nameplate pool the same frame is legitimately removed and re-added within one
fight, so the drain tore down plates showing live units. Queueing one job now
cancels its opposite for that payload: latest intent wins.

UnregisterFrame returned early for a frame that was only QUEUED for
registration, so the queued job survived and took over, one combat later, a
frame the caller had explicitly opted out of. It now drops the queued entry
before the early returns.

SetEnabled skipped the header's dfClickCastEnabled attribute in combat with no
deferral. The OnEnter snippet reads that attribute to decide whether to run, so
enabling click casting mid-fight left every hover a no-op while the UI reported
it working. Deferred as a new headerEnabled job, drained first.

Also: ShouldBindingLoad treated a binding with no `enabled` field as disabled
while the map grouping, special-action and item paths all treat absent as
enabled. For a key whose bindings all lacked the field -- reachable via profile
import, which normalizes nothing -- the builder dropped every one and the key
got no map entry at all. Absent now means enabled everywhere.

And GROUP_ROSTER_UPDATE is registered at last; this module had no roster event
and relied on a hook plus a login-only scan. PLAYER_SPECIALIZATION_CHANGED is
now unit-filtered to the player, so a raid member respeccing no longer costs a
full ~500-frame sweep and the hover window with it.
…ot bind

Five foreign-frame faults from the audit. All of them left another addon's frame
worse than we found it.

Unnamed frames are now refused outright. HANDLE:SetBindingClick resolves its
target through GetName() and errors on nil, so an anonymous frame can never be a
hover-bind target -- but registering one was destructive rather than merely
useless. ClearBlizzardClickCastFromFrame runs early and has no name check, so it
wiped type1/type2 and every modifier variant, while all three functions that
would have installed our bindings bail on the missing name further down. A
third-party frame came out with no left-click target, no right-click menu, and
nothing in their place, until a reload. If we cannot bind it, we do not touch it.

unit1/unit2 are captured and restored. ClearBlizzardClickCastFromFrame nils them
and nothing put them back, so a frame relying on per-button unit overrides lost
that behaviour for the session.

Mousewheel state is captured and restored. Every apply force-enabled the wheel
and nothing ever disabled it again, so a frame that shipped with the wheel off
started swallowing scroll events -- scrolling over it stopped scrolling the
parent scrollframe.

The pre-existing-entries loop now goes through EnsureRegistered. The eligibility
gate lives there, so that loop adopted anything already parked in
ClickCastFrames before our PLAYER_ENTERING_WORLD with no check whatsoever --
including a non-unit secure button, which is the precise case the gate was
written for after a toy button had its type1 replaced.

The third-party scan's issecretvalue guard now precedes the boolean test rather
than following it. Evaluating a secret value in a condition throws, and this runs
inside a C_Timer callback, so one such frame took out the remainder of the
pattern list AND the ClickCastFrames sweep below it: no third-party registration
at all for the session, from a single silent error at login.
BuildCombinedMacroForBindings builds every clause from `.spellName`, but
findBestSpell can return a MACRO-type binding, which has none. A key carrying
two macro bindings with different target types therefore produced no clauses,
returned nil, and got no entry in the unified map -- completely dead, while the
binding list showed it configured and enabled. The single-binding early returns
hide it; it only bites once a key has two or more bindings that do not collapse
into one category.

Falls back to the single-binding builder for the best candidate instead of
returning nil. A macro that ignores the friendly/hostile split is a compromise;
a key that does nothing at all is a bug.
…focus/assist a global bind

The last two audit findings, both wrong-binds rather than dead-binds.

BuildCombinedMacroForBindings never computed the mounted/flying condition. The
single-binding builder stamps ",nomounted,noflying" into every clause it emits,
so "disable while mounted" worked for every key with ONE binding and silently
did nothing for every key with two or more -- the option appears to work right
up until the key the user cares about happens to have a friendly/hostile split.
Applied as a post-pass over the finished clause list rather than threaded
through ten separate concatenations: one place to be correct, and it covers the
unconditional [] and terminal always-cast forms a per-site edit would have
missed.

Special actions were stored with no globalMacroText, and the hovercast script
skips any entry without macro text. So "focus, with a target fallback" worked
while hovering a frame and did nothing at all while hovering nothing -- despite
the fallback being the entire reason that key needs a global bind.
BuildMacroTextForBinding has had working /focus and /assist branches all along;
nothing ever reached them, because the special-action break fires first.

Only focus and assist gain a global form. target and menu genuinely have none:
/target cannot reach cross-instance players (the reason the native handler is
used on frames at all) and there is no macro equivalent of the unit menu, so
both correctly remain frame-only.
SetupHovercastButtonAttributes wrote attributes nothing could read. It named its
slots with GetVirtualButtonName ("type-shiftmouse3") while the bindings that
actually reach that button are installed by BuildHovercastSetupScript using
GetHovercastSuffix ("type-dfmouseshift3") -- two disjoint namespaces on one
button, so no click or key ever resolved to anything it set. Its clear loop had
the same problem in reverse, clearing type1..5 / spell1..5 / macrotext1..5 which
nothing on that button writes, so its own attributes accumulated untouched. The
button is EnableMouse(false) as well, so it cannot be clicked at all.

Deleting it also closes an unbounded leak: it called AddCombatConditional on the
hovercast button, appending to a dfAttrDriverList that nothing ever unregistered
or cleared -- it grew on every ApplyBindings for the whole session.

The real hovercast path, ApplyGlobalBindings -> BuildHovercastSetupScript, is
untouched.

Also removed a `:gsub("BUTTON", "BUTTON")` that read as deliberate
normalisation and only ever uppercased, and the ACTION_TYPES.FOLLOW branch in
the action-name lookup -- ACTION_TYPES has no FOLLOW member, so that comparison
was `actionType == nil` and a binding with no action type displayed as "Follow
Unit" rather than falling through to "Unknown".

Left in place: the uniqueKeys guard in BuildHovercastSetupScript. It is
redundant (it dedupes the table key it is already iterating by, so it cannot
fire twice) but removing it means unwinding a nested if inside a string-builder
for no functional gain, which is churn risk without benefit.
@Krathe82 Krathe82 changed the title Click casting: register frames that gain their unit after creation Click casting: fix frames that never registered, and the pinned-frame bugs that made it fire Aug 2, 2026
… duplicate rebuild

Three items from review, two of them costs this change set introduced.

GROUP_ROSTER_UPDATE called ApplyBindings immediately. ApplyBindings cancels any
in-flight batch walker and re-wipes the header's override bindings before
restarting from batch 0, so one call per roster event means a burst -- a raid
forming, mass join/leave, role assignment, zone-in -- can restart the sweep
faster than it completes, killing the live hover binds again on every restart.
That is the same dead-key class this work exists to close, reached through a
trigger I added. Now a keyed DeferAfter, the same shape as zoneSettle, so a
burst coalesces into one pass. Nothing is lost by the delay: frames created
during a roster event register through EnsureRegistered and the ClickCastFrames
metatable, not through ApplyBindings, which only refreshes frames already
registered.

The batch tail called RefreshKeyboardBindings after the walk, which rebuilt
every snippet a second time -- the batch now does it inline per frame. Checked
before removing: that function's entire body is one loop over the same registry
calling the same builder, additionally gated on dfKeyboardHandlersSetup, so it
is a strict subset of what the batch already did and nothing else depended on
it running once at the end. Its other six callers are untouched.

Test frames are now excluded from eligibility explicitly. They carry
dfIsDandersFrame "for consistency with live frames", so the unit-less exemption
would have accepted them. In practice they are unreachable -- TestMode never
calls RegisterFrameWithClickCast, RegisterAllFrames only walks header children,
and they set `frame.unit` as a plain field rather than the unit attribute
eligibility reads, so the old test refused them too. But leaving it to the
absence of a caller is not a guard, and they are plain Buttons rather than
secure unit buttons, so they should never be adopted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant